Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show features in documentation #2066

Merged
merged 4 commits into from
Jan 6, 2024
Merged

Show features in documentation #2066

merged 4 commits into from
Jan 6, 2024

Conversation

DerZade
Copy link
Contributor

@DerZade DerZade commented Dec 10, 2023

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.


This PR will improve the crate's docs.rs page by showing everything that is exported (even those things which are not included in the default features). Everything that is behind a feature flag will be marked as such.


For anyone, who wants to try this out locally:

cargo +nightly rustdoc --all-features --open -- --cfg docsrs

Cargo.toml Outdated
@@ -55,6 +55,10 @@ criterion = "0.5.0"
# feature when testing, so `cargo test` works correctly.
jpeg = { package = "jpeg-decoder", version = "0.3.0", default-features = false, features = ["platform_independent"] }

[package.metadata.docs.rs]
all-features = true
Copy link
Contributor

@fintelia fintelia Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to avoid enabling the avif and benchmarks feature or we'll have build issues when docs.rs tries to compile the crate. The one feature we do want to enable however is rayon:

Suggested change
all-features = true
features = ["rayon"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just having looked at the code for a tiny bit

I don't think benchmarks should be an issue since everything that depends on benchmark is behind #[cfg(test)] anyway. A quick local rustdocs build confirmed that or am I missing something here?

Why is aviv a problem? Because it depends on the dav1d system lib? Tbh I don't really have experience with docs.rs and system libraries, but I would hope that sys libraries are not an issue for docs.rs, since there are many crates with system lib dependencies on docs.rs, right? That is just me assuming tho 😅, do you have any experience with that?

Copy link
Contributor

@fintelia fintelia Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't specifically tested, so I'm not 100% sure. But my main concern is around the dav1d crate IIRC needing some combination of ninja-build, meson or nasm at compile time. The docs.rs container image contains some native dependencies but likely not those.

Really we should figure out how to do a test documentation build with an environment matching what docs.rs uses. That would definitively reveal what features do/don't work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So looks like dav1d does not work with docs.rs (see here), which is kind of a shame. I wish they would just handle the docs.rs environment in their build script, like suggested here. Maybe I'll add a PR for the dav1d-sys crate in the next few days (don't know if I'll get to it tho).

Apart from that, there apparently is a way to add system libraries to the docs.rs build environment as well as a way to test the build in the same environment as docs.rs. I'll see if I can find some time to test it in the next few days, but again, I can't make any promises.

@fintelia
Copy link
Contributor

Thanks for this PR! I think the approach is nicer than in #2063

@DerZade
Copy link
Contributor Author

DerZade commented Dec 11, 2023

Thanks for this PR! I think the approach is nicer than in #2063

Oh I didn't realize that there was already a PR. Tbh I didn't even check 🙈. I stumbled upon this because I wanted to use the WebPEncoded, which is not (entirely) documented atm, since it is not included in the default features. Instead of complaining I just thought I'd copy paste the solution from one of my projects open a PR 😇

@DerZade
Copy link
Contributor Author

DerZade commented Dec 13, 2023

For the record, I just opened a PR for dav1d-rs (rust-av/dav1d-rs#90) to fix their docs.rs build.

@fintelia
Copy link
Contributor

fintelia commented Dec 17, 2023

Ok, lets wait for dav1d-rs to release a new version that includes that PR.

I figured out how to test how docs.rs will build a crate:

# Pull the docker container
docker pull ghcr.io/rust-lang/crates-build-env/linux:latest

# Run docker
docker run --name docsrs -it -p 127.0.0.1:8000:8000 ghcr.io/rust-lang/crates-build-env/linux:latest

# Install rust (be sure to select 'nightly')
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh

# Download the crate
git clone ...

# Compile (the 'RUSTDOCFLAGS' and '--all-features' are based on the contents in Cargo.toml)
RUSTDOCFLAGS="--cfg docsrs" cargo rustdoc --all-features

# Launch a webserver to display the docs
cd target/doc/image && python3 -m http.server

@DerZade
Copy link
Contributor Author

DerZade commented Jan 6, 2024

I updated dav1d to the latest version (which works on docs.rs) and added the package.metadata.docs.rs section to the Cargo.toml.

I also tested the docs build with the container as suggested by @fintelia

So I think this PR is ready to be merged :)

@fintelia fintelia merged commit 9979822 into image-rs:master Jan 6, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants